Skip to content

fix(firewall): avoid parsing flags from chain names and quoted values#1275

Open
ashishrase wants to merge 2 commits intopuppetlabs:mainfrom
ashishrase:bugfix/proto-number-to-name-malformed-token
Open

fix(firewall): avoid parsing flags from chain names and quoted values#1275
ashishrase wants to merge 2 commits intopuppetlabs:mainfrom
ashishrase:bugfix/proto-number-to-name-malformed-token

Conversation

@ashishrase
Copy link

@ashishrase ashishrase commented Mar 10, 2026

Summary

Fix firewall rule discovery so flag-like substrings inside chain names and quoted values are not parsed as real iptables options.

Today rule_to_hash in lib/puppet/provider/firewall/firewall.rb parses most attributes by scanning the full raw iptables-save line with regexes. That allows matches to happen in places that are not real option tokens, for example:

  • chain names that end with -p
  • chain names that contain -A
  • quoted values such as comments or other quoted payloads

On affected systems this can cause the parser to manufacture malformed values such as proto => '-p'. Once that happens, process_get receives an invalid proto value and discovery can later fail with:

Unsupported proto number: -p

This change fixes the parser at the source by:

  • requiring token boundaries when matching iptables flags, so options are only parsed when they appear as real tokens
  • continuing to ignore quoted payloads when parsing non-quoted attributes
  • continuing to parse quoted attributes from the original rule text
  • preserving existing behavior for real protocol flags such as -p tcp
  • preserving the existing proto => all fallback when no real protocol is present

It does not change utility.rb and does not change firewallchain.rb.

Additional Context

  • Root cause and the steps to reproduce. (If applicable)
  • Thought process behind the implementation.

Root cause / reproduction

On affected systems, iptables-save output can contain rules where the raw line includes flag-like substrings that are not real option tokens, for example:

  • a chain name ending in -p, followed by a real -p udp
  • a chain name containing -A
  • quoted payloads containing text such as -p -p or ! -p -p

Because rule_to_hash scans the full raw rule line, those substrings can be mistaken for real options. In the failing case, the parser can store proto => '-p', and later in discovery this surfaces as:

Unsupported proto number: -p

This can cause Puppet::Type.type(:firewall).instances to fail during rule discovery and can also break firewallchain resource generation that depends on discovered firewall state.

Thought process behind the implementation

The goal of this change is to fix the root cause in the provider parser rather than make downstream protocol conversion more tolerant of malformed intermediate values.

process_get already handles a missing protocol by defaulting to all, so the correct fix is to stop rule_to_hash from manufacturing invalid protocol values in the first place.

Using token-boundary-aware matching ensures that flags such as -p and -A are only parsed when they appear as real iptables option tokens, not when they appear inside chain names or other arbitrary substrings. Quoted payload masking is still kept for the non-quoted parsing branches so quoted content is not interpreted as flags.

Test Coverage

Also adds provider test coverage for:

  • unit parsing: quoted comments containing -p -p do not set proto
  • unit parsing: quoted comments containing ! -p -p do not set proto
  • unit parsing: single-quoted comment payloads do not set proto
  • unit parsing: a real -p tcp outside quotes still parses as proto => tcp
  • unit parsing: chain names ending in -p do not cause proto => '-p'
  • unit parsing: chain names containing -A do not confuse chain parsing
  • end-to-end parsing: get returns proto => all when no real protocol is present
  • end-to-end parsing: get does not surface -p / ! -p as protocol values
  • end-to-end parsing: rules on chains with names containing -p still parse the real protocol correctly

Related Issues (if any)

No linked issue yet.

Checklist

  • 🟢 Spec tests.
  • 🟢 Acceptance tests:
    Acceptance tests were not run locally because Docker/Litmus provisioning is not available in my environment.
  • Manually verified. (For example puppet agent -t on an affected system)

@ashishrase ashishrase requested a review from a team as a code owner March 10, 2026 14:28
@CLAassistant
Copy link

CLAassistant commented Mar 10, 2026

CLA assistant check
All committers have signed the CLA.

@ashishrase ashishrase force-pushed the bugfix/proto-number-to-name-malformed-token branch from 3b535d0 to 3492bb8 Compare March 11, 2026 12:07
@ashishrase ashishrase changed the title Make proto_number_to_name tolerant of malformed non-numeric proto tokens Fix firewall rule discovery so quoted values are not scanned as real iptables options. Mar 13, 2026
@ashishrase ashishrase force-pushed the bugfix/proto-number-to-name-malformed-token branch 2 times, most recently from 55b1ed8 to 726bef0 Compare March 14, 2026 08:02
@ashishrase ashishrase closed this Mar 14, 2026
@ashishrase
Copy link
Author

Need few more test to be done

@ashishrase ashishrase changed the title Fix firewall rule discovery so quoted values are not scanned as real iptables options. fix(firewall): avoid parsing flags from chain names and quoted values Mar 17, 2026
@ashishrase ashishrase reopened this Mar 17, 2026
@ashishrase
Copy link
Author

What’s in this change:
firewall.rb: boundary-aware regex parsing, plus quote masking for both single-quoted and double-quoted payloads
firewall_private_get_spec.rb: covers double quotes, single quotes, chain names ending in -p, and chain names containing -A
firewall_output_parsing_spec.rb: end-to-end get coverage for the same cases
Basically fix for firewall: require token boundaries when parsing iptables-save output

Make rule discovery in firewall.rb treat iptables options as real tokens
instead of matching flag-like substrings anywhere in the rule text.

This fixes cases where rule_to_hash could misread `-p` from chain names
ending in `-p` or from quoted payloads, producing malformed values such
as `proto => '-p'` and later failing with:

  Unsupported proto number: -p

The change keeps existing fw_ naming intact, updates rule parsing to use
token boundaries, preserves quoted-value parsing for attributes that are
meant to read quoted payloads, and adds provider specs for both unit and
end-to-end discovery.
@ashishrase ashishrase force-pushed the bugfix/proto-number-to-name-malformed-token branch from 19fec76 to b8de7fd Compare March 17, 2026 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants